-
Notifications
You must be signed in to change notification settings - Fork 337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
improvement: Add separate option for decorations for evidence params #6219
base: main
Are you sure you want to change the base?
Conversation
4559f71
to
da88569
Compare
val allImplicit = providedArgs.isEmpty || providedArgs.forall { | ||
case Ident(name) => name == nme.MISSING | ||
case _ => false | ||
fun.typeOpt.widen.paramNamess.headOption.map {paramNames => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a need for widening ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, without it GivenAlias
test fails
given (using Xg): Yg with
def doY = "7"
val y = given_Yg<<(using Xg)>> // fun.typeOpt has no params, we need to widen
mtags/src/main/scala-3/scala/meta/internal/pc/PcInlayHintsProvider.scala
Show resolved
Hide resolved
mtags/src/main/scala-3/scala/meta/internal/pc/PcInlayHintsProvider.scala
Outdated
Show resolved
Hide resolved
|object O { | ||
| implicit val i: Int = 123 | ||
| def test[T: Ordering](x: T)(y: T)/*: Nothing<<scala/Nothing#>>*/ = ??? | ||
| test/*[Int<<scala/Int#>>]*/(1)(2)/*(using Int<<scala/math/Ordering.Int.>>)*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally this should be
test/*[Int<<scala/Int#>>]*/(1)(2)/*(using Int<<scala/math/Ordering.Int.>>)*/
test/*[Int<<scala/Int#>>]*/(1)(2)/*(using Ordering.Int<<scala/math/Ordering.Int.>>)*/
But I guess this is not possible with current shortened type printer. I recommend creating an issue for this, so we don't forget to change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's connected to the printer, this PR only adds an option to decouple decorations for context bounds arguments from other implicit arguments. I will create an issue and link it here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
da88569
to
d3a6581
Compare
@@ -260,6 +261,16 @@ object UserConfiguration { | |||
|shown in the hover. | |||
|""".stripMargin, | |||
), | |||
UserConfigurationOption( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also do the same trick as with type, where we have minimal
as an option. So for showImplicitArguments
minimal would not show evidence, however with true it would
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really a fan of this solution in inferred types
.
I would rather have a separate option for inferred type parameters, and use true
/minimal
for showing type annotations in bind:
val x = List(1) match
case head :: tail => 1
case _ => 0
// with 'minimal'
val x: Int = List(1) match
case head :: tail => 1
case _ => 0
// with 'true' (currently also with minimal)
val x: Int = List(1) match
case head: Int :: tail: List[Int] => 1
case _ => 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @jkciesluk, doing it this way makes it far more flexible. At some point we can consider better options for the user to configure those settings but that is more of a client problem.
def unapply(tree: Tree): Option[(List[Symbol], Position, Boolean)] = | ||
def unapply( | ||
tree: Tree | ||
): Option[(List[(Symbol, Boolean)], Position, Boolean)] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it'd be nice to have a dedicated class for (Symbol, Boolean)
, e.g.
sealed trait ImplicitParamSym
case class Param(sym: Symbol) extends ImplicitParamSym
case class ContextBound(sym: Symbol) extends ImplicitParamSym
just because Boolean in itself is quite meaningless. But it's up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe:
sealed trait ImplicitParamSym
case class Param(sym: Symbol) extends AnyVal with ImplicitParamSym
case class ContextBound(sym: Symbol) extends AnyVal with ImplicitParamSym
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe:
sealed trait ImplicitParamSym case class Param(sym: Symbol) extends AnyVal with ImplicitParamSym case class ContextBound(sym: Symbol) extends AnyVal with ImplicitParamSym
This doesn't compile, I used @kasiaMarek solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does compile for me, what is an error you are getting. I would really use AnyVal in such cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, no:
sealed trait ImplicitParamSym extends Any
case class Param(sym: Symbol) extends AnyVal with ImplicitParamSym
case class ContextBound(sym: Symbol) extends AnyVal with ImplicitParamSym
should work
@@ -260,6 +261,16 @@ object UserConfiguration { | |||
|shown in the hover. | |||
|""".stripMargin, | |||
), | |||
UserConfigurationOption( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @jkciesluk, doing it this way makes it far more flexible. At some point we can consider better options for the user to configure those settings but that is more of a client problem.
"""|When this option is enabled, each place where a context bound is used has it | ||
|displayed either as additional decorations if they are supported by the editor or | ||
|shown in the hover. | ||
|""".stripMargin, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be good to include examples here and the client later
def unapply(tree: Tree): Option[(List[Symbol], Position, Boolean)] = | ||
def unapply( | ||
tree: Tree | ||
): Option[(List[(Symbol, Boolean)], Position, Boolean)] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe:
sealed trait ImplicitParamSym
case class Param(sym: Symbol) extends AnyVal with ImplicitParamSym
case class ContextBound(sym: Symbol) extends AnyVal with ImplicitParamSym
d3a6581
to
07c82e5
Compare
07c82e5
to
448ea81
Compare
@@ -616,6 +616,7 @@ class Compilers( | |||
userConfig().showInferredType.contains("true"), | |||
implicitParameters = userConfig().showImplicitArguments, | |||
implicitConversions = userConfig().showImplicitConversionsAndClasses, | |||
contextBounds = userConfig().showEvidenceParams, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move all the settings to a single object first.
No description provided.